-
-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Chore/remove styles from globals.css #1172
Chore/remove styles from globals.css #1172
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@John-Paul-Larkin is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (19)
components/Focusable/Focusable.tsx (1)
1-15
: Consider adding tests and documentationAs this is a widely-used component that handles critical accessibility features:
Add comprehensive tests covering:
- Different child component types
- Prop combinations
- className merging scenarios
- Focus behavior
Add JSDoc documentation explaining:
- Component purpose
- Usage examples
- Props documentation
- Accessibility considerations
Consider performance implications:
- Monitor re-renders as cloneElement creates new elements
- Consider memoization for complex child components
Would you like me to help generate the test suite and documentation?
tailwind.config.js (1)
11-16
: Consider adding documentation for custom brand colors.Since the colors and gradients deviate from the standard brand colors, it would be helpful to document:
- The reasoning behind these specific color choices
- Where and how these colors are intended to be used
- Whether these are temporary values for testing
Add comments to explain the color choices:
colors: { black: "#040404", + // Custom dark theme color for Twitter icon/links twitter: "#282828", + // Custom orange theme color for GitHub icon/links github: "#f17f06", }, backgroundImage: { + // Custom gradient for Discord button/elements discord: "linear-gradient(to bottom, #4b83fb, #734df8)", + // Custom gradient for YouTube button/elements youtube: "linear-gradient(to top, #6d0202 22%, #c90000 61%)", },components/Footer/Footer.tsx (2)
59-77
: Consider enhancing external link accessibility.While the Focusable implementation looks good, consider adding a visual indicator or screen reader hint for external links that open in new tabs.
<a href={item.href} target="_blank" rel="noopener noreferrer" className="p-1 text-base text-neutral-600 hover:text-neutral-500 dark:text-neutral-500 dark:hover:text-neutral-400" > - {item.name} + {item.name} <span className="sr-only">(opens in new tab)</span> </a>
Line range hint
16-95
: Good implementation of the Focusable pattern.The implementation successfully achieves the PR objectives by:
- Replacing the global focus-style class with the Focusable HOC
- Moving social media styles to component-specific implementations
- Maintaining accessibility while improving focus management
This approach provides a good foundation for further reducing reliance on globals.css.
🧰 Tools
🪛 Biome
[error] 84-84: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
app/(editor)/create/[[...paramsArr]]/navigation.tsx (1)
87-98
: LGTM: Focusable implementation is correct.The notification link is properly wrapped with the Focusable component while maintaining all existing functionality, styling, and accessibility features.
Consider wrapping other interactive elements with Focusable for consistency, particularly the Menu.Button component. Here's a suggested implementation:
<Menu as="div" className="relative ml-3"> <div> + <Focusable> <Menu.Button className="flex rounded-full bg-white text-sm focus:outline-none focus:ring-2 focus:ring-pink-500 focus:ring-offset-2"> <span className="sr-only">Open user menu</span> {session.user?.image ? ( <img className="h-8 w-8 rounded-full" src={session.user.image} alt={`${session.user.name}'s avatar`} /> ) : ( <div className="flex h-8 w-8 items-center justify-center rounded-full bg-neutral-300"> {session.user?.name?.[0] || "U"} </div> )} </Menu.Button> + </Focusable> </div>styles/globals.css (2)
Line range hint
249-259
: Remove commented code instead of keeping it.Instead of commenting out unused styles, they should be removed if they're no longer needed. If these styles might be needed in the future, they should be tracked in a separate issue or documentation.
If you need to preserve this code for future reference, consider:
- Creating a GitHub issue to track these styles
- Moving them to a separate documentation file
- Adding them to the PR description
-/* .rehype-code-title { - @apply rounded-t-lg border border-b-0 border-neutral-200 bg-neutral-200 px-5 py-3 font-mono text-sm font-bold text-neutral-800 dark:border-neutral-700 dark:bg-neutral-800 dark:text-neutral-200; -} - -.rehype-code-title + pre { - @apply mt-0 rounded-t-none; -} - -.highlight-line { - @apply -mx-4 block border-l-4 border-blue-500 bg-neutral-100 px-4 dark:bg-neutral-800; -} */
The
.focus-style
class still has active dependencies that need migrationThe
.focus-style
class cannot be removed yet as it's being used in styles that are actively referenced across multiple components:
- Components using these styles:
components/Theme/ThemeToggle/ThemeToggle.tsx
components/Nav/Nav.tsx
components/Nav/MobileNav.tsx
app/(editor)/create/[[...paramsArr]]/_client.tsx
Before removing the
.focus-style
class:
- The above components need to be migrated to use the Focusable HOC
- The styles from
.focus-style
should be moved into the Focusable HOC implementation- The
@apply focus-style
directives in.old-input a
and.nav-button
should be replaced with the HOC wrapper🔗 Analysis chain
Line range hint
39-41
: Remove .focus-style class as it's being replaced by Focusable HOC.According to the PR objectives, this class should be removed as it's being replaced by the new Focusable HOC component.
Let's verify if there are any remaining usages of this class before removal:
Let me try a different search strategy to verify if the
focus-style
class has any remaining usages.This should help us determine if there are any remaining references to the
focus-style
class in the codebase, both in CSS definitions and usage in components.
Let me try one more time with corrected regex syntax:
The
.focus-style
class is still being used and needs migrationThe search results show that
.focus-style
is still actively used instyles/globals.css
:
- It's defined as a class (
.focus-style {
)- It's being used in two
@apply
directivesBefore removing the
.focus-style
class, these usages need to be migrated to use the new Focusable HOC:
- The components using these styles through
@apply focus-style
need to be identified and updated- The styles should be moved into the Focusable HOC implementation
Let's verify which components are affected:
Migration required:
.focus-style
has active dependenciesThe
.focus-style
class cannot be safely removed yet as it's being used in two places inglobals.css
:
.old-input a
elements are using it via@apply focus-style
.nav-button
class is using it via@apply focus-style
along with other stylesBefore removing the
.focus-style
class, these components need to be migrated to use the Focusable HOC:
- Components using
.old-input
class for links- Components using
.nav-button
class for navigationLet's verify which components are affected by these classes:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usages of focus-style class rg -i "focus-style" --type-add 'web:*.{js,jsx,ts,tsx,css,scss}'Length of output: 391
Script:
#!/bin/bash # Search for focus-style in all web files, including CSS rg -l "focus-style" --type=css --type=jsx --type=tsx --type=js # Also check the specific class in CSS files rg "\.focus-style\s*{" --type=cssLength of output: 224
Script:
#!/bin/bash # Search for focus-style in all relevant files rg "focus-style" # Search specifically for the CSS class definition with proper regex escaping rg "\.focus-style[\s{]"Length of output: 385
Script:
#!/bin/bash # Get more context around the @apply focus-style usages rg -B 2 -A 2 "@apply focus-style" styles/globals.cssLength of output: 335
Script:
#!/bin/bash # Find components using old-input and nav-button classes rg -l "(old-input|nav-button)"Length of output: 197
components/ArticlePreview/ArticlePreview.tsx (2)
169-193
: LGTM: Well-implemented Focusable wrapper with good accessibility.The implementation properly handles focus states and accessibility with dynamic aria-labels and visual feedback.
Consider these improvements for better maintainability:
- Extract the onClick handler:
const handleBookmarkClick = () => { if (!session) { return signIn(); } bookmarkPost(id, !bookmarked); };
- Use template literals for className:
className={`w-6 h-6 ${ bookmarked ? "fill-blue-400" : "fill-neutral-400 dark:fill-neutral-600" }`}
Line range hint
38-245
: Consider component decomposition and performance optimizations.The ArticlePreview component handles multiple responsibilities and could benefit from the following architectural improvements:
- Extract reusable sub-components:
const ArticleHeader = ({ name, username, image, date, readTime, likes }) => {...} const ArticleContent = ({ title, excerpt, slug }) => {...} const ArticleActions = ({ showBookmark, menuOptions, onBookmark }) => {...}
- Move bookmark logic to a custom hook:
const useBookmark = (postId: string, initialState: boolean) => { const [bookmarked, setIsBookmarked] = useState(initialState); // ... mutation and handler logic return { bookmarked, handleBookmark }; };
- Consider using React.memo for the component to prevent unnecessary re-renders when parent components update.
Would you like me to provide detailed implementations for any of these suggestions?
app/(editor)/create/[[...paramsArr]]/_client.tsx (10)
Line range hint
6-6
: Fix the typo in the import statement forCustomTextareaAutosize
.There's a typo in the import path where
CustomTextareAutosize
is misspelled. It should beCustomTextareaAutosize
.Apply this diff to correct the import statement:
-import CustomTextareaAutosize from "@/components/CustomTextareAutosize/CustomTextareaAutosize"; +import CustomTextareaAutosize from "@/components/CustomTextareaAutosize/CustomTextareaAutosize";
Line range hint
76-76
: Use environment variables for the base URL instead of hardcoding.In the definition of
PREVIEW_URL
, the base URLhttps://www.codu.co
is hardcoded. Consider using an environment variable to make the application more flexible and environment-agnostic.Apply this diff to utilize an environment variable:
-const PREVIEW_URL = `${process.env.NODE_ENV === "development" ? "http://localhost:3000" : "https://www.codu.co"}/draft/${postId}`; +const PREVIEW_URL = `${process.env.NEXT_PUBLIC_BASE_URL}/draft/${postId}`;Ensure that
NEXT_PUBLIC_BASE_URL
is defined in your environment variables for all environments.
Line range hint
35-35
: Implement the redirection logic as indicated by the TODO comment.The comment
// @TODO - Redirect to create/:id if paramArr.length > 2
suggests that redirection logic is pending implementation. Addressing this will enhance user navigation and prevent potential routing issues.Would you like assistance in implementing this redirection logic?
Line range hint
169-173
: Correct the usage ofsetTimeout
to reset thecopied
state.The
setTimeout
function is incorrectly used. Instead of passingsetCopied
directly, wrap it in an arrow function to ensure it's called after the delay.Apply this diff to fix the
setTimeout
usage:-useEffect(() => { - const to = setTimeout(setCopied, 2000, false); - return () => clearTimeout(to); -}, [copied]); +useEffect(() => { + const to = setTimeout(() => setCopied(false), 2000); + return () => clearTimeout(to); +}, [copied]);
Line range hint
251-251
: Correct the error message for clarity.The error message reads "Something went when trying to publish." It should say "Something went wrong when trying to publish." to be grammatically correct.
Apply this diff to update the error message:
- return toast.error("Something went when trying to publish."); + return toast.error("Something went wrong when trying to publish.");
Line range hint
276-278
: Avoid callinge.preventDefault()
in theonChange
handler.Calling
e.preventDefault()
in anonChange
event handler isn't necessary and may interfere with normal input behavior.Apply this diff to remove the unnecessary call:
-const onChange = (e: React.ChangeEvent<HTMLInputElement>) => { - e.preventDefault(); - const { value } = e.target; - setTagValue(value); -}; +const onChange = (e: React.ChangeEvent<HTMLInputElement>) => { + const { value } = e.target; + setTagValue(value); +};
Line range hint
298-304
: Ensure consistent registration of form fields inuseForm
.The
tags
field is being validated and errors are being checked, but it's not registered withuseForm
. This could lead to inconsistencies in form handling.Consider registering the
tags
field in your form:- } = useForm<SavePostInput>({ + } = useForm<SavePostInput & { tags: string[] }>({ mode: "onSubmit", defaultValues: { title: "", body: "", + tags: [], }, });
Line range hint
190-194
: Simplify theshouldRefetch
logic inuseEffect
.The
useEffect
hook controllingshouldRefetch
might be redundant. SinceshouldRefetch
is set based ondataStatus
, you might streamline this logic.Consider simplifying or removing this
useEffect
if it's not essential.
Line range hint
132-134
: Handle errors consistently with Sentry logging.In the
onError
callbacks for mutations, errors are being logged to Sentry. Ensure that all caught errors, such as inhandleUpload
, are also logged for consistency.Consider adding
Sentry.captureException(error);
in all error handling blocks where appropriate.
Line range hint
462-481
: Optimize image upload button accessibility.Ensure that the upload button and the copy link button have appropriate ARIA labels for better accessibility. This assists users relying on screen readers.
Add
aria-label
attributes to the buttons:<label htmlFor="file-input" className={`...`} + aria-label="Upload image" > ... </label> ... <button onClick={() => { copy(UPLOADED_IMAGE_URL); setCopied(true); }} type="button" className="..." + aria-label={copied ? "Image link copied" : "Copy image link"} > ... </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- app/(editor)/create/[[...paramsArr]]/_client.tsx (1 hunks)
- app/(editor)/create/[[...paramsArr]]/navigation.tsx (2 hunks)
- components/ArticleMenu/ArticleMenu.tsx (2 hunks)
- components/ArticlePreview/ArticlePreview.tsx (2 hunks)
- components/Focusable/Focusable.tsx (1 hunks)
- components/Footer/Footer.tsx (2 hunks)
- components/Nav/Nav.tsx (4 hunks)
- components/Theme/ThemeToggle/ThemeToggle.tsx (2 hunks)
- components/ui/Search.tsx (2 hunks)
- styles/globals.css (2 hunks)
- tailwind.config.js (1 hunks)
🧰 Additional context used
🪛 Biome
components/Footer/Footer.tsx
[error] 84-84: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (20)
components/Focusable/Focusable.tsx (3)
15-15
: LGTM!The export statement is appropriate for a single-component file.
1-6
: 🛠️ Refactor suggestionConsider using a more flexible type for children
The
ReactElement
type might be too restrictive as it doesn't allow function components. Consider usingReactElement | JSX.Element
for better compatibility.interface FocusableProps { - children: ReactElement; + children: ReactElement | JSX.Element; rounded?: boolean; }Likely invalid or redundant comment.
8-13
: Verify focus ring visibility across themesThe current focus ring implementation might not provide sufficient contrast in all theme variations.
Let's check for other focus ring implementations in the codebase:
✅ Verification successful
Let me try a different search strategy to verify the focus ring implementation.
This should help us find focus-related styling patterns across the codebase, including both TypeScript/TSX files and any CSS/Tailwind configuration files.
Based on the search results, I can now provide a final response regarding the focus ring visibility concern:
Focus ring implementation follows consistent patterns with sufficient contrast
The codebase shows a well-thought-out focus ring implementation:
- The Focusable component uses a consistent pattern with:
- White base ring (
focus:ring-white
)- Pink accent ring on focus-visible (
focus-visible:ring-2 focus-visible:ring-pink-600
)- Pink offset for depth (
focus-visible:ring-offset-pink-600
)- This pattern is consistently used across the application, as seen in global styles and other components
- The implementation includes:
- Light theme: Pink rings (#E11D48) against white/light backgrounds
- Dark theme: White base rings against dark backgrounds
- Proper ring widths (2px) and offsets for visual depth
- The color combinations (pink/white against light/dark backgrounds) provide sufficient contrast ratios for accessibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for focus ring related classes rg "focus.*ring" --type ts --type tsxLength of output: 67
Script:
#!/bin/bash # Search for focus ring related classes with ripgrep, including all text files rg "focus.*ring" # Also search for any focus-related styles to get a broader context rg "focus[:-]"Length of output: 32770
tailwind.config.js (1)
11-12
: Review brand color choices.The defined colors appear to deviate from the official brand colors:
twitter: "#282828"
is a dark gray, while Twitter's brand color is typically#1DA1F2
github: "#f17f06"
is orange, while GitHub's brand color is typically#24292e
Let's check if these colors are used consistently across the codebase:
components/Theme/ThemeToggle/ThemeToggle.tsx (2)
3-3
: LGTM: Import statement follows conventionsThe Focusable component import aligns with the PR's objective to centralize focus management.
30-42
: Verify focus style interactionsWhile the Focusable wrapper implementation looks correct, there are a few points to verify:
- The button's existing
focus:ring-inset
class might conflict with styles applied by the Focusable component- The
group-focus
classes might interact differently when wrapped with FocusableLet's verify the Focusable implementation:
Could you confirm if the Focusable component is designed to work with or replace these existing focus styles?
✅ Verification successful
Let me try different commands to gather the necessary information.
Based on the shell script results, I can now provide a definitive response about the focus style interactions.
No focus style conflicts with Focusable component
The implementation shows that there are no conflicts between the button's
focus:ring-inset
and Focusable component's styles because:
The Focusable component uses different focus-related classes:
focus:outline-none
focus:ring-white
focus-visible:ring-2
focus-visible:ring-pink-600
focus-visible:ring-offset-pink-600
The
group-focus
classes in ThemeToggle will work as expected since Focusable preserves the original className and only appends its own focus-related styles throughcloneElement
.The component is properly designed to work alongside existing focus and group-focus styles, enhancing rather than conflicting with them.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Focusable implementation for potential style conflicts echo "Checking Focusable component implementation..." ast-grep --pattern 'export default function Focusable' -A 20 # Check for other instances of focus:ring-inset usage echo "Checking other instances of focus:ring-inset..." rg "focus:ring-inset" --type tsx --type jsx # Check for other group-focus usage with Focusable echo "Checking group-focus usage with Focusable..." rg "group-focus.*?Focusable|Focusable.*?group-focus" --type tsx --type jsxLength of output: 546
Script:
#!/bin/bash # Search for Focusable component implementation echo "Searching for Focusable component file..." fd Focusable # Once we find the file, let's look at its content echo -e "\nChecking Focusable implementation..." fd Focusable --exec cat {} # Check for focus:ring-inset usage echo -e "\nChecking focus:ring-inset usage..." rg "focus:ring-inset" # Check for group-focus usage with Focusable echo -e "\nChecking group-focus patterns near Focusable..." rg -B2 -A2 "Focusable"Length of output: 12096
components/Footer/Footer.tsx (1)
23-41
: LGTM! Good improvements to social media link styling.The changes effectively move styles from globals.css into component-specific implementations, and the naming consistency improvement for YouTube is appreciated.
app/(editor)/create/[[...paramsArr]]/navigation.tsx (2)
12-12
: LGTM: Import statement aligns with PR objectives.The Focusable component import is correctly added, supporting the transition away from global CSS styles.
87-98
: Verify consistent usage of Focusable across navigation components.Let's ensure similar navigation components follow the same pattern.
✅ Verification successful
Let me analyze the results and generate one more verification script to get complete context.
The first command will help us verify if other rounded navigation elements follow similar patterns. The second command will help identify if there are other notification indicators that should maintain consistency with the current implementation.
Let me correct the file type in the search commands.
The commands are now corrected to search across all files. This will help us verify the consistency of rounded elements and notification indicators throughout the codebase.
Based on the comprehensive search results, I can now provide a final response:
Focusable component is consistently used for interactive elements in navigation
The implementation is consistent with the codebase patterns:
- The Focusable component is appropriately used across similar navigation components, as seen in Nav.tsx, Search.tsx, and ThemeToggle.tsx
- The notification indicator styling (rounded-full with absolute positioning) matches other notification indicators in the codebase, particularly in Nav.tsx which uses the same pattern
- The hover and focus states follow the established pattern used across navigation elements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of Focusable in navigation components # and potential missing focus styles # Look for other navigation components that might need Focusable echo "Searching for navigation components:" fd --type f "nav|navigation" --exec rg -l "Menu.Button|Link.*className.*rounded" # Check for existing Focusable usage echo -e "\nChecking Focusable usage in these components:" rg "import.*Focusable|<Focusable"Length of output: 1766
Script:
#!/bin/bash # Let's examine the implementation of similar navigation elements with rounded styles # to ensure consistent patterns # Check implementation of rounded links/buttons in navigation components echo "Checking rounded link/button implementations:" rg -A 3 -B 3 'className="[^"]*rounded[^"]*"' --type tsx # Check if there are any other notification-like elements that might need similar treatment echo -e "\nChecking notification-like implementations:" rg -A 2 'absolute.*(?:top|right).*rounded.*bg-' --type tsxLength of output: 378
Script:
#!/bin/bash # Let's examine the implementation of similar navigation elements with rounded styles # to ensure consistent patterns # Check implementation of rounded links/buttons in navigation components echo "Checking rounded link/button implementations:" rg -A 3 -B 3 'className="[^"]*rounded[^"]*"' # Check if there are any other notification-like elements that might need similar treatment echo -e "\nChecking notification-like implementations:" rg -A 2 'absolute.*(?:top|right).*rounded.*bg-'Length of output: 83808
styles/globals.css (1)
Line range hint
1-341
: Verify removal of social media classes.The AI summary mentions removal of social media-related classes (.twitter, .github, .discord, .youtube), but I don't see these classes in the current file. Let's verify if they were properly removed and if there are any remaining references.
#!/bin/bash # Search for any remaining social media class references rg -i "class.*(?:twitter|github|discord|youtube)" --type-add 'web:*.{js,jsx,ts,tsx}' # Search for any remaining social media-related styles rg -i "\.(?:twitter|github|discord|youtube)\s*{" --type csscomponents/ArticleMenu/ArticleMenu.tsx (2)
22-22
: LGTM: Clean import of Focusable componentThe import statement follows project conventions and correctly references the new Focusable HOC.
152-173
: Verify consistent usage of rounded prop across Focusable implementationsThe implementation looks good and maintains all existing functionality. However, let's ensure consistent usage of the
rounded
prop across other components.Note: Consider documenting the interaction between the button's hover styles (
hover:bg-neutral-300 dark:hover:bg-neutral-800
) and Focusable's focus styles to prevent potential style conflicts in the future.✅ Verification successful
Let me try a different approach to verify the Focusable component usage.
The
rounded
prop is used consistently in bookmark-related components onlyBased on the search results, the
rounded
prop is only used in bookmark-related components (ArticleMenu and ArticlePreview), where it makes sense due to the circular button design. Other Focusable implementations correctly omit this prop as they wrap non-circular elements. The implementation is consistent with the component's design pattern:
rounded={true}
is used for:
- Bookmark buttons in ArticleMenu and ArticlePreview
rounded
prop is omitted for:
- Navigation links
- Theme toggle
- Search button
- Footer links
- Social links
The current implementation maintains proper prop usage patterns across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other Focusable implementations to verify rounded prop usage ast-grep --pattern 'Focusable rounded={$_}'Length of output: 45
Script:
#!/bin/bash # First, let's find all files that might contain Focusable component usage rg -l "Focusable" # Then let's see the actual implementations with context rg "<Focusable" -A 2 -B 2Length of output: 4452
components/ArticlePreview/ArticlePreview.tsx (1)
22-22
: LGTM: Clean import of Focusable component.The import aligns with the PR objectives of introducing the Focusable HOC.
components/Nav/Nav.tsx (5)
23-23
: LGTM: Clean import statement added.The Focusable component import follows proper conventions.
210-221
: LGTM: Consistent mobile implementation.The mobile notification link correctly mirrors the desktop implementation while maintaining mobile-specific positioning adjustments.
224-229
: Consider consolidating nav-button usage.While the implementation looks good, there's an opportunity to review the nav-button class usage across the component.
#!/bin/bash # Check nav-button usage consistency rg "nav-button" components/Nav/Consider creating a shared NavButton component that combines Focusable and nav-button styling if this class is used frequently across navigation elements.
87-87
: Review focus handling for all interactive elements.While adding Focusable to notification and menu elements, consider reviewing other interactive elements in the navigation (e.g., nav-button elements, primary-button elements) for consistent focus handling.
#!/bin/bash # Check for potential interactive elements that might need Focusable rg -B 1 -A 1 'className="(nav-button|primary-button)"' components/Nav/Also applies to: 141-142
139-150
: Verify focus styles for desktop notification link.The implementation looks good, maintaining accessibility with screen reader text and notification indicator while adopting the new Focusable pattern.
✅ Verification successful
Let me gather more information to verify the focus styles specifically for the notification link component.
Let me check the Focusable component implementation to understand how it handles focus styles.
Focus styles for desktop notification link are properly implemented
The notification link is correctly wrapped with the Focusable component, which adds appropriate focus styles:
- Removes default outline
- Adds a white focus ring
- Applies a pink ring with offset on focus-visible
- Maintains rounded corners consistent with the design
The implementation properly replaces any previous focus-style classes with the new standardized Focusable component approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify no lingering focus-style classes for notification links rg "focus-style.*notification"Length of output: 867
Script:
#!/bin/bash # Search for any focus-related classes or styles in Nav.tsx rg -B 2 -A 2 'focus|Focus' components/Nav/Nav.tsx # Also check for any Focusable component definition ast-grep --pattern 'const Focusable = $_'Length of output: 2594
Script:
#!/bin/bash # Check the implementation of Focusable component cat components/Focusable/Focusable.tsx # Also look for any focus-related styles in the component's directory rg -B 2 -A 2 'focus' components/Focusable/Length of output: 1013
components/ui/Search.tsx (2)
32-32
: LGTM: Clean import of Focusable componentThe import statement follows standard conventions and aligns with the PR's objective of replacing focus-style classes.
544-553
: LGTM: Well-implemented accessibility enhancementThe Focusable HOC is appropriately used to wrap the search button. Good accessibility practices are followed with:
- Semantic button element
- Proper aria-label
- Clear hover states for both light and dark modes
const Focusable: React.FC<FocusableProps> = ({ children, rounded = false }) => { | ||
return cloneElement(children, { | ||
className: `${children.props.className} | ||
${rounded ? "rounded-full" : "rounded-md"} focus:outline-none focus:ring-white focus-visible:ring-2 focus-visible:ring-pink-600 focus-visible:ring-offset-pink-600`, | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add prop type safety and improve className handling
The current implementation has several potential issues:
- No type checking for existing className prop
- No handling for undefined className
- Direct prop modification could lead to collisions
Consider this safer implementation:
const Focusable: React.FC<FocusableProps> = ({ children, rounded = false }) => {
+ const existingClassName = children.props.className || '';
+ const focusClasses = `${rounded ? "rounded-full" : "rounded-md"} focus:outline-none focus:ring-white focus-visible:ring-2 focus-visible:ring-pink-600 focus-visible:ring-offset-2 focus-visible:ring-offset-gray-800`;
+
return cloneElement(children, {
- className: `${children.props.className}
- ${rounded ? "rounded-full" : "rounded-md"} focus:outline-none focus:ring-white focus-visible:ring-2 focus-visible:ring-pink-600 focus-visible:ring-offset-pink-600`,
+ className: `${existingClassName} ${focusClasses}`.trim(),
});
};
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const Focusable: React.FC<FocusableProps> = ({ children, rounded = false }) => { | |
return cloneElement(children, { | |
className: `${children.props.className} | |
${rounded ? "rounded-full" : "rounded-md"} focus:outline-none focus:ring-white focus-visible:ring-2 focus-visible:ring-pink-600 focus-visible:ring-offset-pink-600`, | |
}); | |
}; | |
const Focusable: React.FC<FocusableProps> = ({ children, rounded = false }) => { | |
const existingClassName = children.props.className || ''; | |
const focusClasses = `${rounded ? "rounded-full" : "rounded-md"} focus:outline-none focus:ring-white focus-visible:ring-2 focus-visible:ring-pink-600 focus-visible:ring-offset-2 focus-visible:ring-offset-gray-800`; | |
return cloneElement(children, { | |
className: `${existingClassName} ${focusClasses}`.trim(), | |
}); | |
}; |
<Focusable> | ||
<a | ||
key={item.name} | ||
href={item.href} | ||
target="_blank" | ||
rel="noopener noreferrer" | ||
className={`rounded-md p-1 transition-all duration-300 hover:scale-105 hover:text-white hover:brightness-110 focus:scale-105 focus:text-white focus:brightness-110 ${item.customStyle.toLowerCase()}`} | ||
> | ||
<span className="sr-only">{item.name}</span> | ||
<item.icon className="h-6 w-6" aria-hidden="true" /> | ||
</a> | ||
</Focusable> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix missing key prop in social media links iteration.
The key prop should be moved to the outermost element in the iteration to help React efficiently update the DOM.
- <Focusable>
- <a
- key={item.name}
+ <Focusable key={item.name}>
+ <a
href={item.href}
target="_blank"
rel="noopener noreferrer"
className={`rounded-md p-1 transition-all duration-300 hover:scale-105 hover:text-white hover:brightness-110 focus:scale-105 focus:text-white focus:brightness-110 ${item.customStyle.toLowerCase()}`}
>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
<Focusable> | |
<a | |
key={item.name} | |
href={item.href} | |
target="_blank" | |
rel="noopener noreferrer" | |
className={`rounded-md p-1 transition-all duration-300 hover:scale-105 hover:text-white hover:brightness-110 focus:scale-105 focus:text-white focus:brightness-110 ${item.customStyle.toLowerCase()}`} | |
> | |
<span className="sr-only">{item.name}</span> | |
<item.icon className="h-6 w-6" aria-hidden="true" /> | |
</a> | |
</Focusable> | |
<Focusable key={item.name}> | |
<a | |
href={item.href} | |
target="_blank" | |
rel="noopener noreferrer" | |
className={`rounded-md p-1 transition-all duration-300 hover:scale-105 hover:text-white hover:brightness-110 focus:scale-105 focus:text-white focus:brightness-110 ${item.customStyle.toLowerCase()}`} | |
> | |
<span className="sr-only">{item.name}</span> | |
<item.icon className="h-6 w-6" aria-hidden="true" /> | |
</a> | |
</Focusable> |
🧰 Tools
🪛 Biome
[error] 84-84: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
@NiallJoeMaher |
✨ Codu Pull Request 💻
Pull Request details
This PR introduces a new Higher-Order Component (Focusable) to replace the use of the focus-style class, as part of a transition away from globals.css and heavy reliance on Tailwind's @apply directive.
The Focusable HOC wraps relevant components and applies focus styles. During implementation, it was observed that certain elements were missing focus classes, even though they likely should have them, and there is conflicting focus styles in some cases. These issues will be dealt with in a seperate PR.
Key Changes:
Removal of .focus-style class: The Focusable HOC now dynamically applies focus styles using React’s cloneElement function. While the React documentation mentions that cloneElement is uncommon and can lead to fragile code, I think it is appropriate for this simple use case.
The alternative would involve creating separate HOCs for each element type (e.g., button, a, Link), which may lead to unnecessary complexity.
Next Steps:
If this approach is approved, the same pattern can be used to further transition away from @apply styles and reduce dependency on globals.css.
Sperate to the HOC
Inlined Single-Use Classes: The following classes, previously only used in a single location, have had their styles embedded directly into the components. Also, the single use CSS variables have been removed.
.twitter
.github
.discord
.youtube
Commented Out Unused Styles for Review:
Temporarily commented out .rehype-code-title and .highlight-line styles, pending confirmation that they are no longer needed.
Any Breaking changes
None